feat: add xcm benchmarks #1129

ashutoshvarma · 2024-01-09T23:19:41Z

Supersedes - #1012

Pull Request Summary
This PR adds the XCM Benchmarks (for both generic and fungible instructions). This will replace the fixed unit cost we charge currently.

XCM Benchmarks

The benchmarks for each instructions are implemented in pallet-xcm-benchmarks pallet which is separated into fungibles (instr that deals with asset transfer, etc) and generic.
Some of the benchmarks are needed to be modified to properly benchmark xcm in our runtimes, for this a new crate astar-xcm-benchmarks is introduced which is a wrapper over pallet-xcm-benchmarks with some benchmarks modified.
The WrappedBenchmark struct is used to merge benchmarks.

`astar-xcm-benchmarks` structure

├── fungible 
│   ├── benchmarking.rs        # `pallet_xcm_benchmakrs::fungible` modified benchs 
│   └── mod.rs
├── generic
│   ├── benchmarking.rs        # `pallet_xcm_benchmakrs::generic` modified benchs 
│   └── mod.rs
├── lib.rs
├── mock.rs                    # common mock runtime for bench tests

Notes

We only benchmark the fungible instructions with pallet-assets as assets since they involve additional DB read in XcAssetConfig for converting location to asset-id. Therefore they will be more expensive then native asset (pallet_balances)
The fungible's reserve_asset_deposited benchmarks was not available till v1.x.x release therefore is manually copy-pasted for now.

TODO

re-run benchmarks after final review

Check list

added or updated unit tests
updated Astar official documentation
added OnRuntimeUpgrade hook for precompile revert code registration
added benchmarks & weights for any modified runtime logics.

ashutoshvarma · 2024-01-10T14:26:52Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-10T15:13:16Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7476342157.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

ashutoshvarma · 2024-01-10T15:27:40Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-10T15:32:06Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7477119023.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

shaunxw · 2024-01-10T15:37:59Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-10T16:19:25Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7477119023.

shaunxw · 2024-01-10T16:43:40Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-10T16:46:45Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7478071828.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

shaunxw · 2024-01-10T16:49:32Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-10T16:49:55Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7478138231.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

Dinonard

I checked some of the instructions, just to see why certain weights were measured. But I'll probably do more later before the merge. All looks good to me so far.

Just one question about the approach to reuse.

Dinonard · 2024-01-12T08:04:22Z

pallets/astar-xcm-benchmarks/src/generic/benchmarking.rs

+    /// We need re-write buy_execution benchmark becuase our runtime
+    /// needs 1 additional DB read (XcAssetConfig) for fetching unit per sec
+    /// for a fungible asset. The upstream benchmark use native assets thus
+    /// won't accout for it.


One general question about this approach - how viable is to reuse code from the official repo if we have to account for different actions for some calls?

This can change in the future, upstream, and it means we should be re-checking all of the benchmarks each time we do an uplift?

It would seem easier to just have all the benchmarks in our repo since that way we have full control over it. If I understood the current approach correctly, it's highly likely we miss something in the future.

The upstream benchmarks are quite generic themselves, but not generic enough maybe because not many parachains are using them.
The benchmarks we had to re-write were only due the use of hardcoded native asset inside some benchmarks.

buy_execution: hardcoded native asset used for buying execution

transfer_*: does not take ED into account, they use native asset (pallet_balances) which defaults to allow death unlike fungibles (pallet_assets).

expect_pallet: hardcoded pallet index

Other than that all benchmarks were configurable through the pallet's Config. As long as we we used same release upstream pallet (XcmConfig is not changed) it should be fine.

For the hardcoded cases, we can make a upstream PR to make it more generic for other parachains to use as well.

I'm fine either ways, if we copy-paste all benchmarks then we need to keep them updated during uplift if there are major changes like precompile-utils and if we don't copy-paste then we still need to check them during uplifts

Thanks for the explanation!

Let's keep it like this, as you suggested.
For the upstream PR, let's start with an issue first perhaps?

Dinonard · 2024-01-12T08:16:13Z

runtime/shibuya/src/weights/xcm/fungible.rs

+	fn reserve_asset_deposited() -> Weight {
+		// Proof Size summary in bytes:
+		//  Measured:  `0`
+		//  Estimated: `0`
+		// Minimum execution time: 3_440_000 picoseconds.
+		Weight::from_parts(3_603_000, 0)
+	}


Added manually?

No, I meant added (copy-pasted) the benchmark itself from pallet-xcm-benchmark v1.x.x release not the weights

I know, but I asked because there are no docs above the function like the others 🙂

Oh that is because ReserveAssetDeposited does not use touch storage and just put asset into holding thus purely ref_time based computation therefore no storage access logs above

PierreOssun

LGTM

PierreOssun · 2024-01-12T11:18:17Z

primitives/src/xcm/mod.rs

@@ -53,6 +53,7 @@ use pallet_xc_asset_config::{ExecutionPaymentRate, XcAssetLocation};
 mod tests;

 pub const XCM_SIZE_LIMIT: u32 = 2u32.pow(16);
+pub const MAX_ASSETS: u32 = 64;


Is this const not already set somewhere ? Or it's an arbitrary value

It was already set in XcmConfig::MaxAssetsIntoHolding as ConstU64<64>, I just refactor it to be primitives.

It's more of a const than a primitive though 🙂

Might be worth putting all consts into a separate module later on.

ashutoshvarma · 2024-01-12T13:52:59Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-12T13:53:25Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7503066669.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 121a505

github-actions · 2024-01-12T14:40:41Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7503066669.

ashutoshvarma · 2024-01-13T09:39:01Z

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

github-actions · 2024-01-13T09:39:24Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7511661871.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: d258dd8

github-actions · 2024-01-13T10:26:43Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7511661871.

Dinonard

LGTM

I'd suggest to apply the 2nd comment, but the 1st one is more of a question so can be freely ignored.

Dinonard · 2024-01-15T07:09:27Z

runtime/shibuya/src/weights/xcm/fungible.rs

+	/// Storage: ParachainInfo ParachainId (r:1 w:0)
+	/// Proof: ParachainInfo ParachainId (max_values: Some(1), max_size: Some(4), added: 499, mode: MaxEncodedLen)
+	/// Storage: PolkadotXcm SupportedVersion (r:1 w:0)
+	/// Proof Skipped: PolkadotXcm SupportedVersion (max_values: None, max_size: None, mode: Measured)
+	/// Storage: PolkadotXcm VersionDiscoveryQueue (r:1 w:1)
+	/// Proof Skipped: PolkadotXcm VersionDiscoveryQueue (max_values: Some(1), max_size: None, mode: Measured)
+	/// Storage: PolkadotXcm SafeXcmVersion (r:1 w:0)
+	/// Proof Skipped: PolkadotXcm SafeXcmVersion (max_values: Some(1), max_size: None, mode: Measured)
+	/// Storage: ParachainSystem HostConfiguration (r:1 w:0)
+	/// Proof Skipped: ParachainSystem HostConfiguration (max_values: Some(1), max_size: None, mode: Measured)
+	/// Storage: ParachainSystem PendingUpwardMessages (r:1 w:1)
+	/// Proof Skipped: ParachainSystem PendingUpwardMessages (max_values: Some(1), max_size: None, mode: Measured)


I find this part a bit weird - shouldn't these be covered already by the XCM-related hooks?

I find it weird too, but I checked in polkadot & system parachains xcm weights and they also have them - https://github.com/polkadot-fellows/runtimes/blob/187781f7606960d1a61d74a5c2c12d78dfe98bf4/system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs#L75-L89

Dinonard · 2024-01-15T07:11:19Z

runtime/shibuya/src/weights/xcm/fungible.rs

+	}
+}
+
+// For backwards compatibility and tests


Is this needed given it's Shibuya-specific benchmark?

Good point, I'll remove them

github-actions · 2024-01-15T08:25:40Z

Package	Line Rate	Branch Rate	Health
pallets/dapp-staking-migration/src	39%	0%	❌
pallets/ethereum-checked/src	48%	0%	❌
pallets/dapp-staking-v3/src	76%	0%	➖
pallets/astar-xcm-benchmarks/src	0%	0%	❌
pallets/dapps-staking/src/pallet	85%	0%	✔
primitives/src	57%	0%	➖
pallets/astar-xcm-benchmarks/src/fungible	0%	0%	❌
pallets/unified-accounts/src	84%	0%	✔
pallets/collator-selection/src	69%	0%	➖
pallets/xvm/src	40%	0%	❌
pallets/xc-asset-config/src	53%	0%	➖
pallets/dapp-staking-v3/src/benchmarking	0%	0%	❌
precompiles/sr25519/src	64%	0%	➖
precompiles/unified-accounts/src	100%	0%	✔
primitives/src/xcm	66%	0%	➖
precompiles/dapp-staking-v3/src/test	0%	0%	❌
precompiles/dapps-staking/src	94%	0%	✔
pallets/dynamic-evm-base-fee/src	81%	0%	✔
chain-extensions/pallet-assets/src	56%	0%	➖
chain-extensions/unified-accounts/src	0%	0%	❌
precompiles/xcm/src	72%	0%	➖
chain-extensions/types/assets/src	0%	0%	❌
pallets/block-rewards-hybrid/src	87%	0%	✔
pallets/dapp-staking-v3/src/test	0%	0%	❌
precompiles/dapp-staking-v3/src	90%	0%	✔
pallets/astar-xcm-benchmarks/src/generic	0%	0%	❌
precompiles/xvm/src	74%	0%	➖
pallets/dapp-staking-v3/rpc/runtime-api/src	0%	0%	❌
pallets/static-price-provider/src	61%	0%	➖
chain-extensions/types/xvm/src	0%	0%	❌
chain-extensions/types/unified-accounts/src	0%	0%	❌
pallets/inflation/src	70%	0%	➖
chain-extensions/xvm/src	0%	0%	❌
pallets/dapps-staking/src	81%	0%	✔
precompiles/assets-erc20/src	81%	0%	✔
precompiles/substrate-ecdsa/src	74%	0%	➖
Summary	70% (3254 / 4679)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

ashutoshvarma added 6 commits January 10, 2024 03:06

feat: add generic xcm benchmarks wrapper

4f45964

feat: add fungible xcm benchmarks wrapper

292d47f

feat: add xcm benchmarks to shibuya

4d20c03

feat: add missing benchmark

e050241

feat: use WeightInfoBounds in shibuya

4cf8638

Merge branch 'master' into feat/xcm-benchmarks

60c1e93

ashutoshvarma added shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Jan 9, 2024

fix: use max assets properly

0a6acaa

ashutoshvarma marked this pull request as ready for review January 10, 2024 09:04

AstarNetwork deleted a comment from github-actions bot Jan 10, 2024

ashutoshvarma added 4 commits January 11, 2024 13:33

feat: update weights from benchmarks

9d3ece7

feat: merge mocks

3a8254a

feat: refactor code

8b0b9fe

fix: make clippy happy

9399e5f

ashutoshvarma requested review from Dinonard, shaunxw and PierreOssun January 11, 2024 10:20

ashutoshvarma added 2 commits January 11, 2024 18:41

fix: buy execution benchmarks

4a84734

chore: remove file

121a505

Dinonard reviewed Jan 12, 2024

View reviewed changes

PierreOssun previously approved these changes Jan 12, 2024

View reviewed changes

fix: benchmakrs

d258dd8

ashutoshvarma dismissed PierreOssun’s stale review via d258dd8 January 12, 2024 19:35

feat: update weights from benchmarks

ec7587f

asggWa approved these changes Jan 13, 2024

View reviewed changes

Dinonard previously approved these changes Jan 15, 2024

View reviewed changes

feat: apply suggestions

69b84fa

ashutoshvarma dismissed Dinonard’s stale review via 69b84fa January 15, 2024 08:00

Dinonard approved these changes Jan 15, 2024

View reviewed changes

ashutoshvarma merged commit ffe8628 into master Jan 15, 2024
8 checks passed

ashutoshvarma deleted the feat/xcm-benchmarks branch January 15, 2024 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add xcm benchmarks #1129

feat: add xcm benchmarks #1129

ashutoshvarma commented Jan 9, 2024 •

edited

Loading

ashutoshvarma commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

ashutoshvarma commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

Dinonard left a comment

Dinonard Jan 12, 2024

ashutoshvarma Jan 12, 2024

Dinonard Jan 12, 2024

Dinonard Jan 12, 2024

ashutoshvarma Jan 12, 2024

Dinonard Jan 12, 2024

ashutoshvarma Jan 12, 2024

PierreOssun left a comment

PierreOssun Jan 12, 2024

ashutoshvarma Jan 12, 2024

Dinonard Jan 12, 2024

ashutoshvarma commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

ashutoshvarma commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

Dinonard left a comment

Dinonard Jan 15, 2024

ashutoshvarma Jan 15, 2024

Dinonard Jan 15, 2024

ashutoshvarma Jan 15, 2024

github-actions bot commented Jan 15, 2024

feat: add xcm benchmarks #1129

feat: add xcm benchmarks #1129

Conversation

ashutoshvarma commented Jan 9, 2024 • edited Loading

XCM Benchmarks

astar-xcm-benchmarks structure

Notes

TODO

ashutoshvarma commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

ashutoshvarma commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

shaunxw commented Jan 10, 2024

github-actions bot commented Jan 10, 2024

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreOssun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutoshvarma commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

ashutoshvarma commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

github-actions bot commented Jan 13, 2024

Dinonard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 15, 2024

ashutoshvarma commented Jan 9, 2024 •

edited

Loading

`astar-xcm-benchmarks` structure